Open sockets outside of connection objects #275
Conversation
| async with self.request_lock: | ||
| if self.state == ConnectionState.PENDING: | ||
| if not self.socket: | ||
| logger.trace( | ||
| "open_socket origin=%r timeout=%r", self.origin, timeout | ||
| ) | ||
| self.socket = await self._open_socket(timeout) | ||
| self._create_connection(self.socket) | ||
| elif self.state in (ConnectionState.READY, ConnectionState.IDLE): | ||
| pass | ||
| elif self.state == ConnectionState.ACTIVE and self.is_http2: | ||
| pass | ||
| else: | ||
| raise NewConnectionRequired() |
There was a problem hiding this comment.
I'm not sure about why this NewConnectionRequired() case was introduced, so it's possible the new impl is missing something in the HTTP/1.1 case.
There was a problem hiding this comment.
I was also unsure what this was for, a quick glance at the repo and I couldn't find any location where it was caught to be handled.
| backend=self._backend, | ||
| ) | ||
|
|
||
| async def _open_socket( |
There was a problem hiding this comment.
Custom pool implementations would be able to override this method (along with a strict package pin) if they wanted to customize how sockets are opened.
Standalone implementations (single connection w/o pooling) could copy this code and use them in their own open_custom_connection() context manager…
There was a problem hiding this comment.
While you can always have your own implementation you also need to implement the underlying connection code which is not impossible but it would be nice to utilise the existing connection codes in httpcore. By reimplementing all the code you
- Duplicate work, it's IMO a non-trivial amount of code to get a basic HTTP connection going
- Missing out on new functionality/bugfixes that is added by httpcore who know what they are doing when it comes to issues
- Makes the implementers code easier to maintain, they don't need to worry about h11, h2 and how to interact with that
I totally understand this project is in it's infancy so you may not be prepared to expose the existing classes, I just wanted to point out something I wish to see in the future :)
|
|
||
| class AsyncHTTPConnection(AsyncHTTPTransport): | ||
| def __init__( | ||
| self, | ||
| origin: Origin, | ||
| http2: bool = False, | ||
| uds: str = None, | ||
| socket: AsyncSocketStream, |
There was a problem hiding this comment.
This is the change from which all others derive: socket becomes required, and will be passed by whoever wants to interact with an HTTP connection (in our case, the connection pool).
|
Closing as out of date. Fits what we do now, tho. |
See #273 (comment)
Experimenting with a different style for socket management in HTTP connections:
Instead of opening sockets as part of the first
request()call, open the socket beforehand as part of the connection pool'srequest(), and pass it down to the new connection object.